Breaking: Rename gas price parameters - only strk/fri #784
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change refactors the handling of gas price parameters throughout the codebase. It consolidates previously separate gas price fields—distinguished by "wei" and "fri" units for L1 gas price, L1 data gas price, and L2 gas price—into three unified fields: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
FabijanC
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/starknet-devnet-types/src/rpc/gas_modification.rs (1)
36-36: TODO for GasPriceVector converterThere's a TODO comment about implementing a converter to
GasPriceVector. Consider creating a ticket to track this work item to ensure it doesn't get forgotten in future development.crates/starknet-devnet-core/src/starknet/mod.rs (2)
100-104: Clarify the intent behindDUMMY_GAS_PRICE_VECTOR
NonzeroGasPrice::MINtranslates to “1 FRI/WEI”. That non-zero value will propagate to every place where an ETH-denominated gas price is requested (e.g.l1_gas_price(&FeeType::Eth)inrestart_pending_block). In practice this may:
- Produce non-intuitive results (fees of 1 wei on ETH paths) when callers expect ETH prices to be irrelevant or zero.
- Leak into user-facing RPC responses, which could be confusing for tooling that assumes ETH fees are not supported.
Please double-check whether we really want ETH prices to be hard-coded to 1. Alternatives:
-const DUMMY_GAS_PRICE_VECTOR: GasPriceVector = GasPriceVector { - l1_gas_price: NonzeroGasPrice::MIN, - l1_data_gas_price: NonzeroGasPrice::MIN, - l2_gas_price: NonzeroGasPrice::MIN, -}; +/// A placeholder ETH‐price vector. +/// Using `u128::MAX` makes it obvious that ETH prices are unsupported and +/// prevents accidental fee under-estimation. +const DUMMY_GAS_PRICE_VECTOR: GasPriceVector = GasPriceVector { + l1_gas_price: NonzeroGasPrice::new(u128::MAX), + l1_data_gas_price: NonzeroGasPrice::new(u128::MAX), + l2_gas_price: NonzeroGasPrice::new(u128::MAX), +};Or make ETH prices
Option<NonzeroGasPrice>and returnNonewhere ETH is not supported.
493-504: Factor out duplicated gas-price mapping logic
set_block_context_gasre-creates aGasPricesstruct that is almost identical to the one built ininit_block_context. The only moving part is theGasModificationcontainer. Consider extracting a helper such as:fn gas_prices_from(gas: &GasModification) -> GasPrices { … }and re-use it from both places. This eliminates duplication and guarantees that future changes (e.g. a new field in
GasPriceVector) cannot drift out of sync.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/starknet-devnet-core/src/blocks/mod.rs(3 hunks)crates/starknet-devnet-core/src/starknet/add_deploy_account_transaction.rs(0 hunks)crates/starknet-devnet-core/src/starknet/add_invoke_transaction.rs(1 hunks)crates/starknet-devnet-core/src/starknet/add_l1_handler_transaction.rs(0 hunks)crates/starknet-devnet-core/src/starknet/mod.rs(16 hunks)crates/starknet-devnet-core/src/starknet/starknet_config.rs(2 hunks)crates/starknet-devnet-core/src/state/state_diff.rs(1 hunks)crates/starknet-devnet-types/src/rpc/block.rs(0 hunks)crates/starknet-devnet-types/src/rpc/gas_modification.rs(1 hunks)crates/starknet-devnet/src/cli.rs(3 hunks)tests/integration/common/utils.rs(2 hunks)tests/integration/general_integration_tests.rs(2 hunks)tests/integration/test_gas_modification.rs(7 hunks)tests/integration/test_restart.rs(3 hunks)tests/integration/test_simulate_transactions.rs(2 hunks)website/static/devnet_api.json(4 hunks)
💤 Files with no reviewable changes (3)
- crates/starknet-devnet-core/src/starknet/add_deploy_account_transaction.rs
- crates/starknet-devnet-types/src/rpc/block.rs
- crates/starknet-devnet-core/src/starknet/add_l1_handler_transaction.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/integration/test_simulate_transactions.rs (1)
tests/integration/common/utils.rs (4)
assert_contains(648-657)declare_v3_deploy_v3(278-300)felt_to_u128(440-443)get_deployable_account_signer(47-52)
🔇 Additional comments (39)
crates/starknet-devnet-core/src/state/state_diff.rs (1)
269-273: Approved: Simplified gas price configuration.The test has been updated to use the consolidated gas price fields (
l1_gas_price,l1_data_gas_price,l2_gas_price) instead of the previous separate wei/fri fields. This change aligns with the broader refactoring of gas price handling across the codebase.tests/integration/test_simulate_transactions.rs (2)
31-31: Added utility function import correctly.The
felt_to_u128utility function has been properly imported from the common utils module to support the gas price extraction refactoring.
459-463: Improved gas price extraction with standardized conversion.The code now uses the
felt_to_u128utility function to directly convert the gas price fromFelttou128, replacing the previous manual conversion approach. This simplifies the code and makes it more maintainable.tests/integration/common/utils.rs (2)
445-448: Good addition: New utility function for consistent Felt-to-u64 conversion.This helper function follows the same pattern as the existing
felt_to_u128function, providing a consistent approach for conversions fromFeltvalues to native integer types. The implementation correctly extracts the first 8 bytes in little-endian order.
637-643: Improved code maintainability with helper functions.The implementation now uses the helper functions
felt_to_u64andfelt_to_u128for type conversions, making the code more consistent and easier to maintain. This approach standardizes the conversion ofFeltvalues to their respective integer types throughout the codebase.crates/starknet-devnet-core/src/starknet/add_invoke_transaction.rs (1)
430-433: Consolidated gas price fields in test setup.The test setup has been updated to use the three consolidated gas price fields instead of six separate ones, aligning with the broader refactoring of gas price handling across the codebase. This makes the test more consistent with the rest of the implementation.
tests/integration/test_restart.rs (3)
119-119: Variable renaming follows the new gas price structureThe variable renaming from
expected_data_gas_pricetoexpected_l1_data_gas_priceand the corresponding assertion update properly align with the consolidated gas price parameter structure being implemented across the codebase.Also applies to: 122-122
125-130: CLI argument flags updated to match new naming patternThe command line arguments have been properly updated to use the new consolidated gas price flags (
--l1-gas-price,--l1-data-gas-price,--l2-gas-price) instead of the previous-frisuffix variants. This ensures consistent naming across the API.
156-156: Updated assertion fields to match new field namesThe test assertions have been correctly updated to use the new field name
l1_data_gas_priceto ensure the gas price estimates maintain correct values before and after restart.Also applies to: 166-166
tests/integration/general_integration_tests.rs (2)
39-43: Configuration structure updated for gas prices but contains unrelated changeThe JSON configuration object correctly incorporates the consolidated gas price fields (
l1_gas_price,l1_data_gas_price,l2_gas_price). However, I notice thestart_timevalue changed from what was likely 4 to 3, which seems unrelated to the gas price parameter renaming described in the PR objectives.Is the change to
start_timeintended as part of this PR? Since the PR is described as only renaming gas price parameters, this change appears to be unrelated to the stated objective.
72-77: CLI arguments properly updated to use new gas price parameter namesThe CLI arguments passed to the background devnet instance correctly use the new consolidated gas price flags, ensuring consistency with the rest of the codebase changes.
crates/starknet-devnet-core/src/blocks/mod.rs (2)
5-5: Import statements updated to include necessary typesThe imports were correctly updated to include
GasPricefromstarknet_api::blockandGasModificationfromstarknet_types::rpc::gas_modification, which are necessary for the new method implementation.Also applies to: 15-15
398-405: Well-implemented gas modification methodThe new
apply_gas_modificationmethod provides a clean way to update the block header's gas price fields from aGasModificationinstance. This properly centralizes the logic for updating gas prices and supports the broader refactoring of gas price handling throughout the codebase.The implementation correctly maps the consolidated gas price fields to the appropriate fields in the block header, using the proper type conversions.
crates/starknet-devnet-core/src/starknet/starknet_config.rs (2)
109-112: Configuration fields consolidated for cleaner APIThe six separate gas price fields have been consolidated into three clearly named fields:
l1_gas_price,l1_data_gas_price, andl2_gas_price. This makes the configuration more intuitive and reduces redundancy.
155-157: Default implementation correctly updated for consolidated fieldsThe default implementation has been properly updated to initialize the new consolidated gas price fields using the appropriate constants, ensuring backward compatibility with existing code.
tests/integration/test_gas_modification.rs (10)
9-9: Import updated to match the new gas price handlingThe import has been simplified since we now only use
FeltandStarknetErrorfromstarknet_rs_core::types, which aligns with the gas price field consolidation.
21-21: Removed unused importThe
felt_to_u128helper is no longer needed since we're working directly with u128 values for gas prices instead of converting between types.
169-176: Gas price parameters renamed to match new conventionThe gas price parameters in the JSON request are now using the unified naming scheme (
l1_gas_price,l1_data_gas_price,l2_gas_price) instead of separate wei/fri fields. This correctly aligns with the new API schema.
240-242: Simplified gas price type handlingGas prices are now directly converted to u128 from their constants, which simplifies the code by removing the nested type conversions previously needed with the dual wei/fri representation.
247-249: Consistent use of price_in_fri accessorThe test correctly maintains the use of the
.price_in_friaccessor when checking block gas prices, which ensures compatibility with the block structure while using the new unified field names.Also applies to: 266-268, 273-275, 279-281, 297-299
251-258: Gas price variable names updated for clarityVariable names have been updated to clearly indicate they represent FRI prices, making the code more readable and consistent with the new unified field names.
283-291: Consistent naming pattern in second update testThe second update test case follows the same naming pattern as the first, maintaining consistency throughout the test file.
345-346: Simplified gas price JSON structureThe gas price JSON now uses the simplified structure with just one field for L2 gas price, which aligns with the new API schema and makes the test more focused.
354-361: Consistent block gas price assertionsThe assertions for block gas prices correctly use the
.price_in_friaccessor when checking against the default values, maintaining compatibility with existing block structures.
395-399: Final gas price JSON uses unified field namesThe expected final gas price JSON correctly uses the unified field names, which ensures consistency with the new API schema and makes the test more robust.
crates/starknet-devnet-types/src/rpc/gas_modification.rs (3)
8-11: Consolidated gas price fields in GasModificationRequestThe
GasModificationRequeststruct now uses the unified gas price field names (l1_gas_price,l1_data_gas_price,l2_gas_price) instead of separate wei/fri fields. This simplifies the API and makes it more consistent.
17-19: Consolidated gas price fields in GasModificationSimilarly, the
GasModificationstruct has been updated to use the unified gas price field names, ensuring consistency across the codebase.
24-32: Updated update method to use new field namesThe
updatemethod has been refactored to use the new unified field names, maintaining the same functionality while simplifying the code.website/static/devnet_api.json (4)
387-398: API parameters updated to use unified gas price fieldsThe
devnet_setGasPricemethod parameters now use the unified gas price field names (l1_gas_price,l1_data_gas_price,l2_gas_price), all described as being in FRI units. This simplifies the API and makes it more intuitive.
411-425: API result schema updated to match new field namesThe result schema for
devnet_setGasPricehas been updated to match the new unified field names, ensuring consistency between request and response structures.
736-747: Config API schema updated with unified gas price fieldsThe
devnet_getConfigresponse schema now uses the unified gas price field names, ensuring consistency across the API.
790-793: Required fields in config updated to match new namingThe list of required fields in the config API response has been updated to use the new unified field names, maintaining API contract integrity.
crates/starknet-devnet/src/cli.rs (5)
116-121: CLI argument renamed to l1_gas_priceThe CLI argument has been renamed from
gas_price_fritol1_gas_price, which better describes its purpose and aligns with the new unified field naming convention.
123-128: CLI argument renamed to l1_data_gas_priceSimilarly, the data gas price CLI argument has been renamed from
data_gas_price_fritol1_data_gas_price, maintaining consistency with the new naming scheme.
130-135: L2 gas price CLI argument preservedThe L2 gas price CLI argument name was already correct and has been preserved, ensuring compatibility with existing scripts and documentation.
224-226: Updated config struct assignment to use new field namesThe
to_configmethod now uses the new unified field names when creating theStarknetConfigstruct, completing the CLI argument renaming.
550-552: Test environment variables updated to match new CLI flagsThe test environment variables have been updated to use the new unified field names, ensuring that the tests properly verify CLI and environment variable equivalence.
crates/starknet-devnet-core/src/starknet/mod.rs (2)
541-584: Verifyprice_in_weivalues after the refactor
restart_pending_blockpopulatesprice_in_weiusing
block_context.block_info().gas_prices.l1_gas_price(&FeeType::Eth).
Since ETH gas-price vectors are now the dummy constant, every new pending
block advertisesprice_in_wei = 1.If any downstream component still interprets that field literally (e.g.
wallets showing “gas price 1 wei”), the UX may degrade. Please confirm
that the ETH side ofGasPricePerTokenis either ignored or explicitly
documented as a placeholder.
315-316: 👍 Nice use ofapply_gas_modificationSwitching the block-header update to the dedicated helper keeps the block
generation paths consistent and removes bespoke gas mutations from the
call sites. Good job!Also applies to: 340-341
|
Looks like -wei prices might be necessary after all: L1->L2 messages require them, as discussed on Slack. |
Usage related changes
Development related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
New Features
l1_gas_price,l1_data_gas_price, andl2_gas_price.Refactor
Documentation